Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nesting files in subdir for getNewTempFile #30

Merged
merged 3 commits into from
Nov 3, 2023
Merged

nesting files in subdir for getNewTempFile #30

merged 3 commits into from
Nov 3, 2023

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented Nov 3, 2023

Some apps such as docker when installed via snap dont have permissions to access temporary files created directly in /tmp but crucially can access them when nested inside another folder inside /tmp.

This changes getNewTempFile() function to create temporary file inside a nested directory inside the temp location whatever that might be as determined by genTempPath which will honor things like TMPDIR/etc

This resolves docker permission issues but still leaves tmp location to be default as determined by the OS/user

@indecisivedragon was able to test it on her Ubuntu linux instance which has docker installed with snap

@viega
Copy link
Contributor

viega commented Nov 3, 2023

Does it even need to be an option? Or, at least, having it on by default doesn't seem like it could hurt, right?

@miki725
Copy link
Contributor Author

miki725 commented Nov 3, 2023

we could make it the default. guess it was a style choice as paths read funny like /tmp/chalk.abc.file/chalk.xyz.file but yeah we can make it default

@miki725
Copy link
Contributor Author

miki725 commented Nov 3, 2023

updated default behavior

@miki725 miki725 marked this pull request as ready for review November 3, 2023 14:33
@miki725 miki725 requested a review from viega as a code owner November 3, 2023 14:33
@miki725 miki725 changed the base branch from main to dev November 3, 2023 14:38
Copy link
Contributor

@viega viega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think the longer paths are ugly when not needed, I can get behind that too I suppose. Either way, you're good to merge when you want.

@miki725
Copy link
Contributor Author

miki725 commented Nov 3, 2023

it was visually a bit annoying but I think simplicity of usage is more important. otherwise the caller needs to know some specifics about temp files vs rather just "give me temp file". those paths are not meant to be human readable anyhow so its prolly fine

@miki725 miki725 changed the title adding nestInDir option to getNewTempFile nesting files in subdir for getNewTempFile Nov 3, 2023
@miki725 miki725 merged commit 4d031a8 into dev Nov 3, 2023
1 check passed
@miki725 miki725 deleted the ms/tmpfiles branch November 3, 2023 14:43
@viega
Copy link
Contributor

viega commented Nov 3, 2023 via email

@miki725 miki725 restored the ms/tmpfiles branch November 7, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants